Simplify and improve docker setup. #215
Merged
+154
−148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
myronmarston
force-pushed
the
myron/improve-onestep-docker
branch
from
94b99fa to
9c15084
Compare
We'll use this from our Dockerfile to ensure our local gems get used.
These files aren't "primary" enough to put at the repository root. With these changes, the docker command is now: ``` $ docker compose -f config/docker_demo/docker-compose.yaml up --build elasticgraph ```
- Don't `bundle install` or pull in the Ruby base image multiple times. - Instead of copying over the entire project, just copy over the parts that we need. This should make building the `elasticgraph` docker image faster. - Provide a strong guarantee that no released ElasticGraph gems are being used, as we only want our local ElasticGraph gems to get built into the docker image. - Use `ELASTICGRAPH_GEMS_PATH` env var instead of a `sed` command. The `sed` command is a bit harder to maintain, IMO, and could be brittle--the replaced code snippet may change in the future. - Remove OpenSearch Dashboards. While they are often useful to include, we want to keep our demo setup as slimmed down as possible, and minimize the likelihood that users will run out of docker resources. - Honor the `OPENSEARCH_VERSION` ENV var rather than hardcoding it to 2.18.0.
myronmarston
force-pushed
the
myron/improve-onestep-docker
branch
from
9c15084 to
94acf14
Compare
jwils
approved these changes
Feb 23, 2025
There was a problem hiding this comment.
I think the nested directory makes sense as it's a docker file that launches the test app not a dockerfile for the library as I was initially thinking when I started. I think ideally bundle install can run before copying over the rest of the files, and I think a blacklist makes more sense than a whitelist of directories but this is a good improvement from what I had.
As @jwils pointed out, it's beneficial to run the `bundle install` as early as possible, because we want expensive steps that change less frequently (e.g. gem downloads) to run earlier.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow CLI ElasticGraph gem source to be overriden via ENV var.
We'll use this from our Dockerfile to ensure our local gems get used.
Move docker files to
config/docker_demo.These files aren't "primary" enough to put at the repository root.
With these changes, the docker command is now:
Simplify and improve docker setup.
bundle installor pull in the Ruby base image multiple times.This should make building the
elasticgraphdocker image faster.we only want our local ElasticGraph gems to get built into the docker image.
ELASTICGRAPH_GEMS_PATHenv var instead of asedcommand. Thesedcommand is a bit harder to maintain, IMO, and could be brittle--the replaced
code snippet may change in the future.
to keep our demo setup as slimmed down as possible, and minimize the likelihood
that users will run out of docker resources.
OPENSEARCH_VERSIONENV var rather than hardcoding it to 2.18.0.